Remove backend::Backend from runtime-core #1099

syrusakbary · 2019-12-21T06:03:42Z

Description

This PR removes the dependency of a Backend in runtime-core. So it's agnostic and more backends can be plugged in easily.

Why this is important?

By removing backends from wasmer-runtime-core we can make the runtime agnostic, so anyone can plug their own backend into Wasmer without needing to touch the main source code (V8, JavascriptCore, wasm3, ...).
It simplifies the codebase and avoids code leaks from the backend to the runtime API.

Review

Add a short description of the the change to the CHANGELOG.md file

syrusakbary · 2019-12-21T06:03:58Z

bors try

bors · 2019-12-21T06:50:49Z

try

Build failed

wasmerio.wasmer

syrusakbary · 2019-12-21T07:28:20Z

bors try

syrusakbary · 2019-12-21T07:43:48Z

bors r-

syrusakbary · 2019-12-21T07:43:52Z

bors try-

syrusakbary · 2019-12-21T07:43:58Z

bors try

bors · 2019-12-21T09:42:29Z

try

Build succeeded

continuous-integration/travis-ci/push
wasmerio.wasmer

Makefile

MarkMcCaskey

I'll do a full review later but my knee-jerk reaction is that I don't like this change. I do agree that we should have better support for other backends and that the current Backend enum in runtime-core doesn't seem to be flexible enough for what we want. I'll have to think about alternatives more later though

losfair · 2019-12-22T15:14:57Z

lib/clif-backend/src/code.rs

-    fn backend_id() -> Backend {
-        Backend::Cranelift
+    fn backend_id() -> String {
+        "cranelift".to_string()


I think it would be better to have typed backend ids. An approach similar to InternalField for middlewares might work:

wasmer/lib/runtime-core/src/vm.rs

Line 139 in f2f1a08

pub struct InternalField {

We can do this with a wrapper struct and (static) strings too.

I think we'll probably want to have a way to print out the name of the backend in either case, so we'll either need to have the string be part of the id or have a mapping from id to string.

MarkMcCaskey

Here's a list of issues I see from most severe to least severe:

the current changes to the Makefile in the check section remove most of the checking we do -- we can't ship that.
this changes some of the semantics of the crates: we need to document them in any case, but especially with these changes. We may want to add a crate between runtime and runtime-core. edit: after reviewing the PR in depth, I don't see a use for an additional crate right now.
We can improve type safety, in addition or instead of using a static string (described below), by providing a wrapper type -- this is more for our sake than the public API's sake, but I think it will improve readability in some places.
I think &'static str might be a better choice than String. String is a dynamic, runtime value, &'static str is a compile-time string. It seems that being forced to use values you declared at compile-time is less error prone than a dynamic string here. I personally can't think of any case where the string would need to be dynamic right now.

lib/runtime-core/src/vm.rs

src/bin/wasmer.rs

lib/runtime-core/src/backend.rs

syrusakbary · 2020-01-07T15:17:56Z

the current changes to the Makefile in the check section remove most of the checking we do -- we can't ship that.

I just reverted that

We can improve type safety, in addition or instead of using a static string (described below), by providing a wrapper type -- this is more for our sake than the public API's sake, but I think it will improve readability in some places.

I think &'static str might be a better choice than String. String is a dynamic, runtime value, &'static str is a compile-time string. It seems that being forced to use values you declared at compile-time is less error prone than a dynamic string here. I personally can't think of any case where the string would need to be dynamic right now.

Agreed. I talked with @losfair and he thinks we can use a similar strategy as we are doing with Internal Fields (here's a POC: #1106 ).

I think we can merge this, and then merge the other PR apart, or we can do the static str changes meanwhile. Let me know!

MarkMcCaskey · 2020-01-08T17:56:56Z

I think we can merge this, and then merge the other PR apart, or we can do the static str changes meanwhile. Let me know!

Sure, I'm fine merging in in-progress work in general as long as we have plans to follow up and we don't release until the public APIs are in the shape we want them to be.

MarkMcCaskey

The core changes look good -- I'm not fully on board with this being the way the public API looks though but as discussed, we can follow up on that!

One last thought: perhaps it would simplify the code to have the Backend enum always contain all our backends regardless of features and have the conditional compilation logic in other places Not high priority, but because you've just interacted with this code you're in a good position to decide if that kind of change would be helpful.

MarkMcCaskey · 2020-01-08T18:24:44Z

lib/runtime-core/Cargo.toml

@@ -52,7 +52,7 @@ cc = "1.0"
 [features]
 debug = []
 trace = ["debug"]
-# backend flags used in conditional compilation of Backend::variants
+# backend flags no longer used


I'd prefer just deleting them if we're not using them!

MarkMcCaskey · 2020-01-08T18:26:56Z

lib/runtime/Cargo.toml

+features = ["rc"]
+[dependencies.serde_derive]
+version = "1.0"
+


It looks like this chunk of the Cargo.toml came from runtime-core, do you know if we can remove the feature from runtime-core?

Unfortunately both need serde_derive as they use #[derive(Serialize, Deserialize, ...)].

Oops, the git diff shows 4 lines, I intended to point to just the top one with features = ["rc"]!

It's a very minor thing, just thought it'd be better to remove it if it's not needed

lib/runtime/src/lib.rs

syrusakbary · 2020-01-10T12:42:02Z

bors r+

1099: Remove backend::Backend from runtime-core r=syrusakbary a=syrusakbary  # Description This PR removes the dependency of a Backend in runtime-core. So it's agnostic and more backends can be plugged in easily. Why this is important? * By removing backends from wasmer-runtime-core we can make the runtime agnostic, so anyone can plug their own backend into Wasmer without needing to touch the main source code (V8, JavascriptCore, wasm3, ...). * It simplifies the codebase and avoids code leaks from the backend to the runtime API.  # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>

bors · 2020-01-10T13:00:08Z

Build failed

wasmerio.wasmer

Bumps [getrandom](https://github.com/rust-random/getrandom) from 0.1.13 to 0.1.14. - [Release notes](https://github.com/rust-random/getrandom/releases) - [Changelog](https://github.com/rust-random/getrandom/blob/master/CHANGELOG.md) - [Commits](rust-random/getrandom@v0.1.13...v0.1.14) Signed-off-by: dependabot-preview[bot] <[email protected]>

Bumps [parking_lot](https://github.com/Amanieu/parking_lot) from 0.9.0 to 0.10.0. - [Release notes](https://github.com/Amanieu/parking_lot/releases) - [Changelog](https://github.com/Amanieu/parking_lot/blob/master/CHANGELOG.md) - [Commits](Amanieu/parking_lot@0.9.0...0.10.0) Signed-off-by: dependabot-preview[bot] <[email protected]>

Bumps [regex](https://github.com/rust-lang/regex) from 1.3.1 to 1.3.3. - [Release notes](https://github.com/rust-lang/regex/releases) - [Changelog](https://github.com/rust-lang/regex/blob/master/CHANGELOG.md) - [Commits](rust-lang/regex@1.3.1...1.3.3) Signed-off-by: dependabot-preview[bot] <[email protected]>

Bumps [cc](https://github.com/alexcrichton/cc-rs) from 1.0.48 to 1.0.50. - [Release notes](https://github.com/alexcrichton/cc-rs/releases) - [Commits](rust-lang/cc-rs@1.0.48...1.0.50) Signed-off-by: dependabot-preview[bot] <[email protected]>

syrusakbary · 2020-01-13T09:24:10Z

bors r+

1099: Remove backend::Backend from runtime-core r=syrusakbary a=syrusakbary  # Description This PR removes the dependency of a Backend in runtime-core. So it's agnostic and more backends can be plugged in easily. Why this is important? * By removing backends from wasmer-runtime-core we can make the runtime agnostic, so anyone can plug their own backend into Wasmer without needing to touch the main source code (V8, JavascriptCore, wasm3, ...). * It simplifies the codebase and avoids code leaks from the backend to the runtime API.  # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]> Co-authored-by: Syrus Akbary <[email protected]> Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

bors · 2020-01-13T09:50:04Z

Build failed

wasmerio.wasmer

syrusakbary · 2020-01-13T11:05:03Z

bors r+

1099: Remove backend::Backend from runtime-core r=syrusakbary a=syrusakbary  # Description This PR removes the dependency of a Backend in runtime-core. So it's agnostic and more backends can be plugged in easily. Why this is important? * By removing backends from wasmer-runtime-core we can make the runtime agnostic, so anyone can plug their own backend into Wasmer without needing to touch the main source code (V8, JavascriptCore, wasm3, ...). * It simplifies the codebase and avoids code leaks from the backend to the runtime API.  # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]> Co-authored-by: Syrus Akbary <[email protected]> Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

bors · 2020-01-13T11:25:41Z

Build succeeded

continuous-integration/travis-ci/push
wasmerio.wasmer

1143: Set backend_id to static str r=MarkMcCaskey a=syrusakbary  # Description As per feedback in #1099, set backend_id to static str.  # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

syrusakbary requested review from bjfish, Hywan, losfair, MarkMcCaskey and nlewycky as code owners December 21, 2019 06:03

bors bot added a commit that referenced this pull request Dec 21, 2019

Try #1099:

51c3c93

bors bot added a commit that referenced this pull request Dec 21, 2019

Try #1099:

a88600a

bors bot added a commit that referenced this pull request Dec 21, 2019

Try #1099:

a18d83b

syrusakbary commented Dec 21, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

MarkMcCaskey reviewed Dec 22, 2019

View reviewed changes

losfair reviewed Dec 22, 2019

View reviewed changes

MarkMcCaskey reviewed Jan 6, 2020

View reviewed changes

lib/runtime-core/src/vm.rs Outdated Show resolved Hide resolved

src/bin/wasmer.rs Outdated Show resolved Hide resolved

lib/runtime-core/src/backend.rs Show resolved Hide resolved

syrusakbary added 11 commits January 7, 2020 16:10

Remove backend code

303d44c

Move requires pre validation into the ModuleCodeGenerator

d4e9645

Remove Backend dependency

d7154fe

Make all tests pass

18421e3

Remove backend specific features from root Cargo

f3b9ecb

Improved syntax

71be5be

Removed unnecessary checks

0cb3df2

Fixed changelog link

5d2ea93

Fixed lint

ac0c5c9

Make cranelift optional for middleware

e57677b

Fix lint

d36d883

Run formatting

d544f5a

wasmerbot force-pushed the backend-refactor branch from 1cf7ab0 to d544f5a Compare January 7, 2020 15:16

MarkMcCaskey approved these changes Jan 8, 2020

View reviewed changes

syrusakbary added 2 commits January 10, 2020 13:40

Remove unused backend flags

f353ac0

Merge branch 'master' into backend-refactor

9ca9770

Mark McCaskey and others added 9 commits January 13, 2020 10:17

Update the name of experimental IO devices in fs

cc28804

Update the experimental io device path to include /dev/

5b52589

Make all tests pass

aa6189a

Fixed changelog link

4faa78b

Fixed runtime check

846c632

Fixed lint

3892ea8

bors bot merged commit 3892ea8 into master Jan 13, 2020

bors bot deleted the backend-refactor branch January 13, 2020 11:25

syrusakbary mentioned this pull request Jan 13, 2020

Set backend_id to static str #1143

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove backend::Backend from runtime-core #1099

Remove backend::Backend from runtime-core #1099

syrusakbary commented Dec 21, 2019 •

edited

Loading

syrusakbary commented Dec 21, 2019

bors bot commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

bors bot commented Dec 21, 2019

MarkMcCaskey left a comment

losfair Dec 22, 2019

MarkMcCaskey Jan 8, 2020

MarkMcCaskey left a comment •

edited

Loading

syrusakbary commented Jan 7, 2020 •

edited

Loading

MarkMcCaskey commented Jan 8, 2020

MarkMcCaskey left a comment

MarkMcCaskey Jan 8, 2020

MarkMcCaskey Jan 8, 2020

syrusakbary Jan 10, 2020 •

edited

Loading

MarkMcCaskey Jan 10, 2020

MarkMcCaskey Jan 10, 2020

syrusakbary commented Jan 10, 2020

bors bot commented Jan 10, 2020

syrusakbary commented Jan 13, 2020

bors bot commented Jan 13, 2020

syrusakbary commented Jan 13, 2020

bors bot commented Jan 13, 2020

Remove backend::Backend from runtime-core #1099

Remove backend::Backend from runtime-core #1099

Conversation

syrusakbary commented Dec 21, 2019 • edited Loading

Description

Review

syrusakbary commented Dec 21, 2019

bors bot commented Dec 21, 2019

try

Build failed

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

syrusakbary commented Dec 21, 2019

bors bot commented Dec 21, 2019

try

Build succeeded

MarkMcCaskey left a comment

Choose a reason for hiding this comment

losfair Dec 22, 2019

Choose a reason for hiding this comment

MarkMcCaskey Jan 8, 2020

Choose a reason for hiding this comment

MarkMcCaskey left a comment • edited Loading

Choose a reason for hiding this comment

syrusakbary commented Jan 7, 2020 • edited Loading

MarkMcCaskey commented Jan 8, 2020

MarkMcCaskey left a comment

Choose a reason for hiding this comment

MarkMcCaskey Jan 8, 2020

Choose a reason for hiding this comment

MarkMcCaskey Jan 8, 2020

Choose a reason for hiding this comment

syrusakbary Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

MarkMcCaskey Jan 10, 2020

Choose a reason for hiding this comment

MarkMcCaskey Jan 10, 2020

Choose a reason for hiding this comment

syrusakbary commented Jan 10, 2020

bors bot commented Jan 10, 2020

Build failed

syrusakbary commented Jan 13, 2020

bors bot commented Jan 13, 2020

Build failed

syrusakbary commented Jan 13, 2020

bors bot commented Jan 13, 2020

Build succeeded

syrusakbary commented Dec 21, 2019 •

edited

Loading

MarkMcCaskey left a comment •

edited

Loading

syrusakbary commented Jan 7, 2020 •

edited

Loading

syrusakbary Jan 10, 2020 •

edited

Loading